fix(messaging): stop merge tags inserting double quotes - #91352
fix(messaging): stop merge tags inserting double quotes#91352posthog[bot] wants to merge 2 commits into
Conversation
🦔 PostHog Review reviewed this pull requestNothing worth raising this time, so here's a calming picture instead:
|
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| Function | Location | Complexity | Limit |
|---|---|---|---|
<anonymous> |
frontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsx:869 |
12 | 10 |
✅ Bundle size — no change
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.29 MiB · no change
No file changed by more than 1000 B.
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.44 MiB · 22 files | no change | ███░░░░░░░ 32.0% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.88 MiB · 3,262 files | no change | █████████░ 91.4% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 307.0 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 263.5 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.js |
| 258.4 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.7 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.26 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.26 MiB · 18 files | no change | ████░░░░░░ 39.6% of 5.72 MiB |
| Deferred (lazy) | 2.12 MiB · 45 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 752.2 KiB | dist/toolbar/toolbar-app-L24UJTUY.css |
| 590.6 KiB | dist/toolbar/chunk-chunk-D5NX33MU.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-53LW4ZM7.js |
| 134.1 KiB | dist/toolbar/chunk-chunk-BJDPG263.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 71.3 KiB | dist/toolbar/toolbar-app-HYWSRVIM.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-P7THZRDY.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-QXHKE4LO.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +2.3 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1463.26 MiB · 🔺 +2.3 KiB (+0.0%)
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, contained frontend fix to how the merge-tag menu formats a person-property template string, with matrix unit tests covering the new cases; not in risky territory (no auth/billing/crypto/CI/data-model/API-contract surface), and no open reviews or holds block it.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 17L, 1F substantive, 33L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (33L, 2F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ e7a6d3a · reviewed head e7a6d3a |
The PR was retargeted to a different base branch, so the approved diff is no longer what was reviewed. Stamphog re-reviews automatically.
The email templater menu built every person-property merge tag as
{{person.properties["name"]}}. The double quotes come from us, not the
user. Inside an HTML attribute like a link href, a double quote ends the
attribute and mangles the tag. When the quotes survive as an HTML entity,
the renderer resolves the tag to an empty string and the email sends a
blank value with no error.
Emit dot notation for identifier-safe names and single-quoted brackets for
everything else, matching buildDelayExpression in the workflows delay step.
Generated-By: PostHog Desktop
Task-Id: 4b35bbb8-8157-4e92-86f5-f619d4294408
e7a6d3a to
00b5773
Compare
There was a problem hiding this comment.
Contained frontend fix to how merge-tag values are generated, with matching unit tests covering the new helper; no risky-territory surface (no auth, billing, data model, or infra changes) and no unresolved concerns.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 17L, 1F substantive, 33L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (33L, 2F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 00b5773 · reviewed head 00b5773 |
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
…ge-tags-inserting-414661
New commits were pushed — dismissing the stamphog approval from an earlier head. This PR no longer qualifies for automatic review.
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| if (BARE_IDENTIFIER_REGEX.test(name)) { | ||
| return `{{person.properties.${name}}}` | ||
| } | ||
| const escaped = name.replace(/\\/g, '\\\\').replace(/'/g, "\\'") |
There was a problem hiding this comment.
buildPersonPropertyMergeValue() (frontend/src/scenes/hog-functions/email-templater/emailTemplaterLogic.tsx:161-167) only escapes backslash and single-quote characters before embedding the property name inside {{person.properties['...']}}. It does not escape { or }. property.name comes from PropertyDefinitions loaded from loadPersonPropertyDefinitions (line ~486), which reflects arbitrary person-property keys captured from real ingested events — i.e. any unauthenticated visitor hitting the project's public capture endpoint can set a person property whose name contains }} or {{. When an org member later opens the merge-tag menu and inserts that property, the generated string, e.g. for a name like foo}} <img src=x onerror=alert(1)> {{bar, becomes {{person.properties['foo}} <img src=x onerror=alert(1)> {{bar']}}, which is saved verbatim into the stored email template HTML.
At render time, LiquidRenderer.renderWithHogFunctionGlobals (nodejs/src/cdp/utils/liquid.ts) uses the non-greedy regex /\{\{(.*?)\}\}/ to find and entity-decode Liquid tags, then calls liquid.parseAndRenderSync. Because Liquid's tag scanner also looks for the first literal }}, the injected }} prematurely closes the tag; the attacker's HTML/script payload then sits as literal template markup outside any {{ }} expression, so LiquidJS's outputEscape: 'escape' (which only escapes resolved values, not literal template bytes) never touches it — it is emitted unescaped into every email sent from that template, reaching the org's real customers. Since this PR explicitly rewrites the merge-tag emission specifically to close an HTML/Liquid-breakout bug, the new escaping should also neutralize { and } (e.g. reject/strip them, or refuse bracket-notation for names containing them and fall back to a safe placeholder) rather than only guarding against straight double quotes.
Severity: medium | Confidence: 45% | React with 👍 if useful or 👎 if not
1 participant
Problem
{{person.properties["name"]}}, so the double quotes come from us, not the user.href, a double quote ends the attribute and mangles the tag. This produced a paying-customer support ticket, with a Liquid parse error (expected "|" before filter).nodejs/src/cdp/utils/liquid.tsdoes not decode", so the tag resolves to an empty string and the email sends a blank value with no error.Changes
{{person.properties.name}}for identifier-safe names and{{person.properties['name']}}for everything else. A person pasting one into a link no longer breaks the link or sends a blank value.buildPersonPropertyMergeValuehelper emits dot notation for bare identifiers and single-quoted brackets otherwise, matchingbuildDelayExpressionin the workflows delay step. Single quotes are safe inside a double-quoted HTML attribute, so the failure mode goes away.How did you test this code?
Automated only; the local quill workspace was not built, so the editor UI could not be rendered.
emailTemplaterLogic.test.ts— added abuildPersonPropertyMergeValuecase matrix: a bare identifier stays dot-access; a leading-$name, a name with a space, a dotted name, and a name with an apostrophe each go through single-quoted, escaped brackets. This catches a regression that re-introduces double quotes or fails to bracket a non-identifier name — the exact fault in the report.pnpm --filter=@posthog/frontend fixapplied. Typecheck reports no errors in the changed files; the unrelated@posthog/quillmodule-resolution errors are pre-existing in this environment.Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
Authored by Claude Code (Opus 4.8) from an inbox report on the workflow email merge-tag menu. Skills invoked:
/writing-tests,/simplify,/writing-simplified-technical-english.liquid.ts("/%22not decoded); that is left alone, since single quotes never reach the renderer entity-encoded from a double-quoted attribute, so the observed symptom is fixed at the source.buildDelayExpressionfromproducts/workflowsstepDelayLogic, but importing it pulls a kea logic module (connected toworkflowLogic) intofrontend/src, and the frontend rules forbid importing acrossproducts/*. A small pure helper with a pointer comment to the sibling was the cleaner tradeoff.Created with PostHog Desktop from this inbox report.